feat(wallet-toolbox): add BRC-177 noSend expiry - #504
Conversation
|
tonesnotes
left a comment
tonesnotes
left a comment
There was a problem hiding this comment.
I asked grok to look at the PR and summarize the changes + approach to get me started.
My conclusions after reading it over, the idea of creating a pre-signed recovery transaction before sharing a noSend transaction externally (along with an expected validity duration for the external transaction) is a great extension to the original design. Second conclusion, grok is picking nits that may make sense to the implementor but would take me quite a while to have my own opnion.
| throw new WERR_INVALID_OPERATION('BRC-177 protected action changed before signature release') | ||
| } | ||
| if (current.noSendExpiryMode !== 'blockheight' && | ||
| Math.floor(Date.now() / 1000) >= verifyInteger(current.noSendExpiryDeadline)) { |
There was a problem hiding this comment.
[bug] Wall-clock expiry is re-checked inside the commit transaction before unsigned → signed CAS, but blockheight expiry is only checked outside the transaction. A tip that crosses the deadline between outer getHeight() and the CAS can still release a signed target after the height deadline.
Suggestion: Mirror the time-based path: re-validate height in the same atomic section as the CAS, or refuse release unless getHeight() + safetyMargin < deadline / record observed height under CAS.
| } | ||
| const now = Math.floor(Date.now() / 1000) | ||
| if (target.noSendExpiryMode !== 'blockheight' && target.noSendExpiryDeadline <= now) { | ||
| throw new WERR_INVALID_OPERATION('BRC-177 action expired before it could be armed') |
There was a problem hiding this comment.
[suggestion] validateArmSnapshot skips deadline enforcement for blockheight mode, so an action can be armed after its height deadline has already passed.
Suggestion: Reject arming when height is already >= deadline, consistent with wall-clock arm checks.
| if (await markObservedTarget(storage, transaction, true)) result.targetWon++ | ||
| return true | ||
| } | ||
| if (transaction.noSendExpiryState === 'reclaiming') { |
There was a problem hiding this comment.
[suggestion] Once reclaiming, later passes only reconcile races. If the reclaim row is failed while the target stays unknown and never proves, there is no automatic retry/self-heal; the anchor can stay locked.
Suggestion: When target is still explicit unknown, reclaim has no proof, and the anchor is again a conclusive UTXO, reset the reclaim ProvenTxReq to sendable (or return to conflicted and retry) without releasing the anchor on mere rejection.
| transaction.noSendExpiryState === 'revocation-requested' || | ||
| transaction.noSendExpiryState === 'broadcast' || | ||
| transaction.noSendExpiryState === 'conflicted' | ||
| if (stateCanObserve && isKnownOrMined(targetStatus)) { |
There was a problem hiding this comment.
[suggestion] Any service known/mined permanently moves to broadcast and stops new reclaim activation. A false-positive known that never confirms leaves the anchor reserved indefinitely.
Suggestion: Document operator recovery, and/or after a long window without provenTxId allow a conflicted-style resume (fresh unknown + unspent anchor) before treating broadcast as permanent.
| const fundingArgs = makeNoSendExpiryFundingArgs(anchorSatoshis, target.labels) | ||
| fundingArgs.includeAllSourceTransactions = target.includeAllSourceTransactions | ||
| const funding = await createAction(storage, auth, fundingArgs) | ||
| const anchor = funding.outputs.find(output => output.providedBy === 'storage' && output.purpose === 'change') |
There was a problem hiding this comment.
[nit] Funding anchor discovery uses first providedBy===storage && purpose===change then asserts satoshis. Prefer selecting the unique change output with satoshis === anchorSatoshis so ordering changes cannot bind the wrong vout.
2 participants
Program and scope
3dffbdd7a21687373e4aefc605a81252d0167d1fImpact
Affected packages/services and intended patch versions (publication occurs only
through the release workflow after approval):
@bsv/wallet-toolbox@2.11.0@bsv/wallet-toolbox-client@2.11.0@bsv/wallet-toolbox-mobile@2.11.0Verification
pnpm build: passedpnpm test: passed across the complete workspace; Wallet Toolbox 221/221 suites, 2,099 passed and 1 skippedpnpm lint,pnpm format:check,pnpm health:check, wallet typecheck: passedpnpm check-versions,pnpm license:release-check,pnpm license:pack-check: passedpnpm audit:security: passed; two pre-governed high advisories remain ignored, with no dependency or lockfile changepack:checkpassed against clean consumerspackage-release-artifacts.mjscompatibility, public API, artifacts, dependencies, docs, and operations
scope-based skip is expected and validated by the merge gate
Security and dependencies
audit results were reviewed
(including accepted or false-positive issue states) and zero unreviewed hotspots;
Sonar's aggregate
Quality Gate passedverdict alone is not merge evidenceand removal condition
Dependency evidence
No dependency or lockfile changes.
Release and operations
controlling program
Completion evidence
passing checks, resolved alerts, measurements, or an approved exception
guidance are current or concretely not applicable
being handed to another contributor as “complete”
restriction is assumed